Skip to content

Fix #4771: Relax overriding check for polymorphic methods #4782

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 10, 2018

Conversation

smarter
Copy link
Member

@smarter smarter commented Jul 10, 2018

Just like in Scala 2, we now allow an override to widen the bounds of a
polymorphic type parameter (this is sound because parameters can vary
contravariantly). (And just like Scala 2, we continue not allowing an
overriding to widen the type of a term parameter).

@smarter smarter force-pushed the fix/poly-override branch 2 times, most recently from 3ae4c01 to caa452c Compare July 10, 2018 02:04
@allanrenucci allanrenucci requested a review from odersky July 10, 2018 07:34
@smarter smarter force-pushed the fix/poly-override branch from caa452c to ad8e826 Compare July 10, 2018 13:16
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM

// (Code like this is found in the collection strawman)
//
// 2. In the case of two method types or two polytypes with matching
// parameters and implicit status, merge corresppnding parameter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: corresppnding

// 2. In the case of two method types or two polytypes with matching
// parameters and implicit status, merge corresppnding parameter
// and result types.
(tp1, tp2) match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bound to be very inefficient since we do not optimize tupled pattern matches yet. I suggest to use nested matches instead, similar to how it is done in infoJoin.

if ctx.typeComparer.matchingPolyParams(tp1, tp2) =>
tp1.derivedLambdaType(
mergeParamNames(tp1, tp2),
(tp1.paramInfos, tp2.paramInfos).zipped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shorter and more efficient: use zipWithConserve (in Decorators.scala)

if ctx.typeComparer.matchingPolyParams(tp1, tp2) =>
tp1.derivedLambdaType(
mergeParamNames(tp1, tp2),
(tp1.paramInfos, tp2.paramInfos).zipped
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zipWithConserve again

@odersky odersky assigned smarter and unassigned odersky Jul 10, 2018
Just like in Scala 2, we now allow an override to widen the bounds of a
polymorphic type parameter (this is sound because parameters can vary
contravariantly). (And just like Scala 2, we continue not allowing an
overriding to widen the type of a term parameter).
@smarter smarter force-pushed the fix/poly-override branch from ad8e826 to b87e964 Compare July 10, 2018 14:34
@smarter smarter merged commit 385082b into scala:master Jul 10, 2018
@allanrenucci allanrenucci deleted the fix/poly-override branch July 10, 2018 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants